Skip to content

fix(security): gate exam questions, options and lesson comments through their parent (#542) - #553

Merged
guillermoscript merged 1 commit into
masterfrom
fix/rls-exam-child-tables-542
Jul 26, 2026
Merged

fix(security): gate exam questions, options and lesson comments through their parent (#542)#553
guillermoscript merged 1 commit into
masterfrom
fix/rls-exam-child-tables-542

Conversation

@guillermoscript

Copy link
Copy Markdown
Owner

What

Replaces the three USING (true) SELECT policies on exam_questions, question_options and lesson_comments with predicates that route through their parent (exams / lessons), tightens the two sibling FOR ALL staff policies on the exam tables, and converges the #509 authenticated preview-lesson branch with the anon one.

Closes #542

Why

20260313152048_rls_exam_child_tables.sql created exam_questions and question_options as USING (true). #509 replaced the parent exams policy with a tenant + staff/author + has_course_access gate and stopped there, so any authenticated user of any tenant — holding no entitlement, past their school's access_cutoff_at — could read every exam's correct_answer, grading_rubric, ai_grading_criteria, expected_keywords and question_options.is_correct straight over PostgREST. Both the #509 exam gate and the #494 access cutoff were bypassable for the thing exams exist to protect. lesson_comments was the same shape with lower stakes.

Beyond the four policies the issue lists, this also tightens the sibling FOR ALL staff policies on both exam tables. They matched on get_tenant_role() IN ('teacher','admin') with no row predicate at all, and permissive policies OR together — so without this a teacher of tenant A keeps reading (and writing) every other tenant's exam questions through that policy, and the acceptance criterion "a member of tenant A gets zero rows" would fail for any staff account. Verified: before the change a caller presenting a tenant-A teacher claim read all of tenant B's questions; after, zero.

The preview overshoot is a content leak introduced by the #509 hardening: (is_preview AND status='published') omitted the parent-course-published EXISTS its anon counterpart carries, so a teacher drafting a course who marked a lesson preview + published exposed that body to every signed-in tenant member while anonymous visitors correctly could not see it.

How it works

New can_read_exam(_exam_id) helper (SECURITY INVOKER, so auth.uid() / get_tenant_id() / is_tenant_staff() resolve to the caller) mirrors the #509 exams gate plus the pre-existing super-admin policy, and both exam child tables gate on it — question_options two-hop via exam_questions. Per CLAUDE.md these tables have no tenant_id (adding one errors the query — the #282 teacher-grading regression), hence routing through the FK. The staff/author branches are carried over deliberately: the exam editor, teacher grading and the AI grading path in app/actions/exam-grading.ts all read through RLS-bound user clients, so a missing branch breaks grading rather than failing safe.

Also adds the three missing FK indexes (exam_questions.exam_id, question_options.question_id, lesson_comments.lesson_id), which every new predicate looks rows up by.

How to QA

On a fresh npm run db:reset, then npm run dev:

  1. Assert no open policy — should return zero rows:
    select tablename, policyname from pg_policies
     where tablename in ('exam_questions','question_options','lesson_comments')
       and qual = 'true';
  2. Automatednpx playwright test exam-answer-key-rls --workers=1 (10 tests; needs no dev server, it drives PostgREST directly). Covers: cross-tenant read → 0 rows; same-tenant-no-entitlement → 0 rows; unscoped whole-table sweep → 0 rows; staff still read rubrics + is_correct; entitled student still reads the exam they are sitting; comments cross-tenant → 0 / in-tenant → visible; preview lesson of a draft course hidden from both signed-in and anon, restored when the course is published.
  3. Teacher path, by handcreator@codeacademy.com on code-academy.lvh.me:3000, open a course exam in the editor and a submission's grading screen; questions, options and rubrics all render.
  4. AI grading end to endnpx playwright test exam-grading-test --workers=1 as alice@student.com (needs her seeded entitlement to course 2001).

Screenshots / GIF

n/a — no user-visible surface changes; this is an RLS-only change. Evidence is the policy dump in step 1 and the spec in step 2.

Verification

  • npm run typecheck — clean
  • npm run test:unit370/370 (matches the 146a8cfa baseline)
  • npm run build — passes
  • npx eslint tests/playwright/exam-answer-key-rls.spec.ts — clean (repo-wide lint has a large pre-existing baseline, unchanged here; SQL isn't linted)
  • New spec 10/10, stable across repeated runs
  • Prove-the-bug: with the pre-exam_questions and question_options are readable by every authenticated user in every tenant #542 policies restored in place, the 5 leak tests fail and the 5 positive/staff tests still pass — the spec discriminates rather than passing vacuously
  • Role-simulated SQL probes (set_config('request.jwt.claims', …)) confirm each branch: cross-tenant student 0, non-entitled member 0, forged tenant-A teacher claim 0, owning-tenant staff full read, entitled student full read, super admin full read
  • student-exams, evaluations-security, teacher-content — 40 passed
  • exam-grading-test (AI grading end to end) — passes with the new policies active

Two pre-existing failures, confirmed unrelated by re-running each with the old policies restored (identical failure):

  • full-student-journey.spec.ts — targets course 9999 / lessons 10000-10001, which no longer exist in supabase/seed.sql; the page 404s regardless of RLS
  • The earlier exam-grading-test failure was alice@student.com having lost her seeded entitlement to course 2001 (deleted by a parallel job sharing this local database, not by this change); restoring the row makes it pass

Notes for the reviewer

  • Not applied to cloud. Local only, following the pattern of fix(#532): gate checkpoint attempts on entitlements, not enrollments #535. The issue asks for the pg_policies assertion on cloud too — that is a deliberate separate step.
  • lib/database.types.ts not regenerated: this migration adds no tables or columns, only policies, one function and three indexes.
  • The FOR ALL staff policies keep get_tenant_role() rather than swapping to is_tenant_staff() — the change there is purely the added row predicate, to keep the write gate itself untouched.

Checklist

  • npm run typecheck and npm run test:unit pass
  • npm run build passes
  • Every new tenant-scoped query filters by tenant_id (or the table genuinely has no such column — all three here genuinely have none, hence the FK routing)
  • Tested with every relevant role (student / teacher / admin / super admin / anon)
  • Loading and error states handled (n/a — no UI change)
  • New UI strings added to both messages/en.json and messages/es.json (n/a — no new strings)
  • Migration applies cleanly on npm run db:reset, has RLS policies; lib/database.types.ts regeneration not needed (no schema shape change)

🤖 Generated with Claude Code

https://claude.ai/code/session_01Xwf7RJcxHNRVXb3ufFuT8g

…gh their parent (#542)

`exam_questions` and `question_options` were created `USING (true)` by
20260313152048 and left behind when #509 gated their parent `exams`, so any
authenticated user of any tenant — holding no entitlement, past their school's
`access_cutoff_at` — could read every exam's `correct_answer`, `grading_rubric`,
`ai_grading_criteria` and `is_correct` straight over PostgREST.
`lesson_comments` was the same shape with lower stakes.

These tables have no `tenant_id` of their own, so the new predicates route
through the FK to the parent that does, mirroring the #509 `exams` gate
(tenant + staff/author/entitlement) plus the pre-existing super-admin policy
via a shared `can_read_exam()` helper.

Also tightened, beyond the four SELECT policies the issue lists: the sibling
`FOR ALL` staff policies on both exam tables. They matched on
`get_tenant_role() IN ('teacher','admin')` with no row predicate at all, and
permissive policies OR together — so a teacher of tenant A kept reading and
writing every other tenant's exam questions through that policy, and the SELECT
fix alone would have bought nothing for staff accounts.

And converged the #509 authenticated preview branch with the anon one by
requiring the parent course to be published: a teacher drafting a course who
marked a lesson preview + published was exposing that body to every signed-in
member of the tenant while anonymous visitors correctly could not see it.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01Xwf7RJcxHNRVXb3ufFuT8g
@guillermoscript guillermoscript added security Security vulnerability or hardening severity:critical Critical severity security finding db-migration Requires a Supabase migration labels Jul 26, 2026
@guillermoscript

Copy link
Copy Markdown
Owner Author

Browser + live-session QA (local, npm run db:reset state)

Real browser (Playwright), signed-in sessions on code-academy.lvh.me:

Path Client Result
Student exam taker (/courses/2001/exams/2001) as alice admin client + requireCourseAccess question text + True/False options render
Lesson page comment thread as alice RLS-bound browser client thread renders ("1 Comments", seeded body visible)
Teacher exam editor (/teacher/courses/2001/exams/2001) as creator@codeacademy.com RLS-bound user client "Questions (10)", 32 populated fields — question text, options, grading rubric, AI criteria
Teacher grading screen (/submissions/1) as creator@codeacademy.com RLS-bound user client all 10 questions, per-option Correct markers, student answers, Override controls

The last two are the pages that went blank in #282 when a tenant_id filter was wrongly applied to these tables — they are the reason the staff branch is carried over.

Direct PostgREST probe with real user JWTs — the actual attack surface, against a seeded Code Academy exam whose course nobody holds an entitlement for (correct_answer = SECRET-ANSWER-KEY-542):

Caller gated answer key all exam_questions all question_options all lesson_comments
Before — alice (same tenant, not entitled) 🔴 SECRET-ANSWER-KEY-542 11
Beforestudent@e2etest.com (different tenant) 🔴 SECRET-ANSWER-KEY-542 11
After — alice (same tenant, not entitled) ✅ 0 rows 10 (only the exam she is entitled to) 20 0
Afterstudent@e2etest.com (different tenant) ✅ 0 rows 0 0 0
Aftercreator@codeacademy.com (tenant staff) ✅ 1 row, key readable 11 21 0

Staff keep full read of their own tenant; everyone else loses what they were never meant to have.

Gates: typecheck clean · test:unit 370/370 · build passes · exam-answer-key-rls 10/10 · AI grading E2E passes end to end with the new policies.

@guillermoscript
guillermoscript marked this pull request as ready for review July 26, 2026 01:29
@guillermoscript
guillermoscript merged commit 5e468df into master Jul 26, 2026
2 checks passed
guillermoscript added a commit that referenced this pull request Jul 26, 2026
…elative (#541)

The two ledger checks compare cloud against the migrations in the CURRENT
checkout, so on a feature branch a migration another in-flight branch has
already applied reads as an orphan, and one on this branch not yet applied
reads as pending. Neither is drift.

Found while confirming #541: PRs #553 (#542) and #554 (#543) had applied
20260726013256, 20260726015858, 20260726100000 and 20260726110000 to cloud,
none of which exist on this branch — so verify:cloud run here reports four
orphans that are not drift at all.

Left as guidance rather than logic. Filtering by "is this stamp on some other
branch" would need a remote ref walk and would silently excuse the exact
condition the check exists to catch. The orphan detail line now names the
possibility and says to re-check on master; the payment-invariant checks are
branch-independent and meaningful anywhere.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01HPFyHjqzSR6Ku1gyWjeQie
guillermoscript added a commit that referenced this pull request Jul 26, 2026
…ke migration drift detectable (#541) (#552)

* fix(security): apply the transactions INSERT lockdown to cloud and make drift detectable (#541)

The #538 lockdown (20260725180000) was never applied to the cloud project. Until
this commit, `authenticated` still held INSERT on `public.transactions` there and
the INSERT policy was the older #528 shape, so a caller could open a pending row
quoting `settlement_base: 1` against a priced product, pay that on-chain, and
have /api/payments/solana/verify — which verifies against the row's own
settlement_base — flip it to successful and grant the entitlement.

Applied to cloud (verified by querying the live catalog, not by reading files):

  - REVOKE INSERT ON transactions FROM authenticated, anon
    INSERT is now held only by postgres and service_role.
  - INSERT policy re-created with all four settlement_* IS NULL pins.

Nothing depended on the grant: PR #539 had already moved both user-scoped
inserts onto createAdminClient(). The two user-scoped .update() calls that
remain in the checkout route only touch provider_subscription_id and status,
both inside the #528 three-column UPDATE grant, so they are unaffected.

WHY IT WAS LOST, AND WHAT NOW CATCHES IT

Cloud migration stamps had drifted from repo filenames: four migrations were
applied through the MCP apply_migration tool, which stamps a fresh timestamp
instead of the filename. That makes "what is missing from cloud?" unanswerable —
the re-stamped four read as pending, and the genuinely missing fifth was
indistinguishable from them.

  npm run verify:cloud

queries the live database and exits non-zero on drift. It asserts ledger
integrity (every repo migration stamped under its own filename; no stamp
matching no file) and the #512/#528/#538 payment invariants against catalog
state rather than migration text, so a later re-widening fails it too.

The rules are split into scripts/lib/verify-cloud-schema-checks.ts and driven
from both healthy and drifted fixtures in tests/unit/verify-cloud-schema.test.ts
(13 tests). The healthy fixture is the exact state read back from cloud, so the
policy-pin matcher is tested against how Postgres actually renders the
expression. That is also how the "fails if you re-grant INSERT" criterion is
demonstrated: as a repeatable test, rather than by briefly re-opening a write
grant on the payments table of the only production database this project has.

docs/MIGRATIONS.md now states the push-only rule and why, with the incident
table, and reframes the Management API fallback so the schema_migrations stamp
reads as the thing that makes it safe rather than optional bookkeeping.

Gates: typecheck clean, test:unit 383 passed (370 baseline + 13), eslint clean
on new files, build succeeds.

Refs #540

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01HPFyHjqzSR6Ku1gyWjeQie

* fix(db): repair the drifted cloud migration ledger (#541)

Applied to cloud. Re-stamps the four migrations that had been applied through
the MCP apply_migration tool (which stamps a fresh timestamp instead of the
migration's filename) and records 20260725180000, whose DDL was applied earlier
in this issue:

  20260721120000_add_binance_personal_provider        was 20260725213441
  20260725110000_transaction_split_snapshot_backstop  was 20260725213508
  20260725160000_entitlement_gated_enrollment_inserts was 20260725213610
  20260725170000_transactions_column_hardening        was 20260725192946
  20260725180000_transactions_insert_lockdown         was absent

Each of the four was confirmed genuinely live by querying what its DDL created
before being treated as a ledger-only repair, so this re-runs no DDL.

The file is committed under the stamp apply_migration assigned it
(20260726005843) so the repair is not itself an orphan — the ledger now matches
the repo exactly: 171 files, 171 stamps, zero pending, zero orphans.

Idempotent: on a fresh `supabase db reset` the four UPDATEs match nothing and
the INSERT no-ops, since the CLI has already stamped 20260725180000 by the time
this file runs.

verify:cloud now reports 10/10 PASS.

Gates: typecheck clean, test:unit 383 passed.

Refs #540

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01HPFyHjqzSR6Ku1gyWjeQie

* docs(migrations): note that verify:cloud's ledger checks are branch-relative (#541)

The two ledger checks compare cloud against the migrations in the CURRENT
checkout, so on a feature branch a migration another in-flight branch has
already applied reads as an orphan, and one on this branch not yet applied
reads as pending. Neither is drift.

Found while confirming #541: PRs #553 (#542) and #554 (#543) had applied
20260726013256, 20260726015858, 20260726100000 and 20260726110000 to cloud,
none of which exist on this branch — so verify:cloud run here reports four
orphans that are not drift at all.

Left as guidance rather than logic. Filtering by "is this stamp on some other
branch" would need a remote ref walk and would silently excuse the exact
condition the check exists to catch. The orphan detail line now names the
possibility and says to re-check on master; the payment-invariant checks are
branch-independent and meaningful anywhere.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01HPFyHjqzSR6Ku1gyWjeQie

---------

Co-authored-by: Claude Opus 5 (1M context) <noreply@anthropic.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

db-migration Requires a Supabase migration security Security vulnerability or hardening severity:critical Critical severity security finding

Projects

None yet

Development

Successfully merging this pull request may close these issues.

exam_questions and question_options are readable by every authenticated user in every tenant

1 participant